Conversation
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed a5541e29baca3e8fba44b964821e0c1d82dcf317 against 6cae7f5da8911c746cf873581885f90c3820dd96. The generated static module-ID slice preserves the membership and order of TSJS_MODULES, and the affected deferred-module routing and template-fingerprint paths pass their focused tests. No actionable issues found.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Reviewed commit a5541e29baca3e8fba44b964821e0c1d82dcf317.
The generated static module-ID slice preserves the membership and core-first ordering of TSJS_MODULES. Workspace callers use the returned static references correctly in deferred-module lookup and template fingerprinting. No actionable findings.
The exact-head native bundle test suite passed (2 tests), and the build script generated all 13 JS modules. General adapter/build/lint gates rely on remote CI.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- Analyze (javascript-typescript): PASS
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS (required)
- cargo test (axum native): PASS
- Analyze (actions): PASS
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- format-docs: PASS (required)
- vitest: PASS
- Analyze (rust): PASS
- prepare integration artifacts: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- Analyze (javascript-typescript): PASS
There was a problem hiding this comment.
Summary
Correct, focused change: all_module_ids() no longer allocates on the per-request parse_single_module_filename path, and both call-site edits are behaviour-preserving. One structural concern blocks: the codegen now emits two independent module-ID lists that can drift apart silently.
Verified independently on a5541e29b: cargo fmt --check, cargo clippy-fastly, cargo test-axum (26 passed), cargo test -p trusted-server-core … tsjs (49 passed), cargo test -p trusted-server-js (2 passed) — all green.
Neither inline comment carries a one-click
suggestion: the blocking fix also deletes lines in two other diff hunks, and the test addition lands outside any hunk. Both comments carry the proposed code in full with an apply-manually note.
Blocking
🔧 wrench
- Codegen emits two independent module-ID lists that can silently drift — see inline at
crates/trusted-server-js/build.rs:154-161
Non-blocking
🌱 seedling
- Add a regression test pinning the two generated lists together — see inline at
crates/trusted-server-js/src/bundle.rs:19-20
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- Analyze (javascript-typescript): PASS
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS (required)
- cargo test (axum native): PASS
- Analyze (actions): PASS
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- format-docs: PASS (required)
- vitest: PASS
- Analyze (rust): PASS
- prepare integration artifacts: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
…ting Fix a compile error in the const-derived array and add a regression test pinning all_module_ids() to the generated TSJS_MODULES table. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Summary
all_module_ids()built a freshVec<&'static str>on every call even though the answer never changes; it now returns a&'static [&'static str]backed by a build-script-generated static array, so theparse_single_module_filenamelookup on the per-request deferred-module path no longer allocates.build.rsalready generatesTSJS_MODULESas a compile-time array, so extending that codegen to also emitALL_MODULE_IDSkeeps the same pattern and avoids the extra.collect()an iterator-returning version would force onpublisher.rs:1970'sconcatenated_hashcall.clippy::redundant_static_lifetimesfailure this introduced in the generated code, and dropped a redundant&at theconcatenated_hashcall site now thatall_module_ids()already returns a reference.Changes
crates/trusted-server-js/build.rspub(crate) const ALL_MODULE_IDS: [&str; N]array alongsideTSJS_MODULES, written viawriteln!instead of a per-iterationStringallocationcrates/trusted-server-js/src/bundle.rsall_module_ids()is now aconst fnreturning&ALL_MODULE_IDS(&'static [&'static str]) instead of collecting aVeccrates/trusted-server-core/src/publisher.rsparse_single_module_filenamescans the static slice directly (.iter().copied().find(...)) instead of allocating aVecfirst; removed a redundant&on theall_module_ids()call passed toconcatenated_hashCloses
Closes #435
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo clippy -p trusted-server-js --all-targets -- -D warningsandcargo clippy -p trusted-server-core --all-targets -- -D warningsdirectly, pluscargo test -p trusted-server-js(existing bundle tests atbundle.rs:143,157cover the new return type unchanged)Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)